Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for wait while PV reattach #608

Merged
merged 1 commit into from
Jun 9, 2021
Merged

Support for wait while PV reattach #608

merged 1 commit into from
Jun 9, 2021

Conversation

prashanth26
Copy link
Contributor

@prashanth26 prashanth26 commented May 17, 2021 •
edited
Loading

Basic working for support for reattaching works with VolumeAttachments

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #249

Special notes for your reviewer:

Release note:

Draining of pods with PVs (Persistent Volume) now waits for re-attachment of PV on a different node when `volumeAttachments` support is enabled on the cluster. Else it falls back to the default PV reattachment timeout value configured. The default value is `90s` and this can be overwritten via the `machine-pv-reattach-timeout` flag. Please enable permissions to allow listing of `volumeAttachments` resource while importing these changes.

@prashanth26 prashanth26 requested a review from a team as a code owner May 17, 2021 10:09
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels May 17, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 17, 2021
@prashanth26 prashanth26 marked this pull request as draft May 17, 2021 10:11
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 17, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 17, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 17, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 17, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 17, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 18, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 18, 2021
@gardener-robot gardener-robot added the lifecycle/rotten Nobody worked on this for 12 months (final aging stage) label May 19, 2021
AxiomSamarth
AxiomSamarth reviewed May 20, 2021
View reviewed changes
pkg/util/provider/drain/drain.go
pvList := pvMap[i]
vols, err := o.getVolIDsFromDriver(pvList)
for podHash, persistantVolumeList := range pvMap {
persistantVolumeListDeepCopy := persistantVolumeList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to call the DeepCopy() method in this assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value for persistantVolumeList is changing after every run. Hence I wanted to create a copy of the variable to use it for the object I am building here. I hope that looks okay?

@gardener-robot gardener-robot added lifecycle/stale Nobody worked on this for 6 months (will further age) and removed lifecycle/rotten Nobody worked on this for 12 months (final aging stage) labels May 21, 2021
@gardener-robot gardener-robot added lifecycle/rotten Nobody worked on this for 12 months (final aging stage) and removed lifecycle/stale Nobody worked on this for 6 months (will further age) labels Jun 2, 2021
@prashanth26 prashanth26 marked this pull request as ready for review June 2, 2021 05:25
@prashanth26
Copy link
Contributor Author

The PR is ready for review. Minor things for TODOs need to be fixed.
/invite @amshuman-kr

amshuman-kr
amshuman-kr suggested changes Jun 3, 2021
View reviewed changes
Copy link

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @prashanth26! I have some comments below. The main one about the one goroutine per pod.

pkg/util/backoff/wait_test.go Outdated Show resolved Hide resolved
pkg/util/k8sutils/helper.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/drain.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/drain.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/drain.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/volume_attachment.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/volume_attachment.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/volume_attachment.go Outdated Show resolved Hide resolved
pkg/util/provider/drain/volume_attachment.go Outdated Show resolved Hide resolved
pkg/util/provider/machinecontroller/controller.go Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 8, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 8, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 8, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 8, 2021
@prashanth26
Copy link
Contributor Author

/ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@prashanth26
Wait for re-attachment of PVs during serialized eviction of pods
d488fd5 
- Draining of pods with PV (Persistent Volume) now waits for re-attachment of PV on a different node.
- When volumeAttachments support is enabled on the cluster, it tracks volume attachments to determine this.
- Else it falls back to the default PV reattachment timeout value configured. Default value is 3mins.

Co-authored-by: Amshuman K R <[email protected]>
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@prashanth26
Copy link
Contributor Author

/ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 9, 2021
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 9, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 9, 2021
@prashanth26 prashanth26 merged commit c48f10d into gardener:master Jun 9, 2021
@prashanth26 prashanth26 mentioned this pull request Jun 14, 2021
Closed
@prashanth26 prashanth26 deleted the serial-reattach branch June 16, 2021 10:43
This was referenced Jul 14, 2021
Merged
Merged
@prashanth26 prashanth26 mentioned this pull request Jul 28, 2021
Merged
@AxiomSamarth AxiomSamarth mentioned this pull request Aug 11, 2021
Merged
@kon-angelo kon-angelo mentioned this pull request Oct 4, 2021
Merged
@Gerrit91 Gerrit91 mentioned this pull request Oct 19, 2021
Merged
1 task
@himanshu-kun himanshu-kun mentioned this pull request Jul 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amshuman-kr amshuman-kr amshuman-kr approved these changes

@AxiomSamarth AxiomSamarth Awaiting requested review from AxiomSamarth

Assignees
No one assigned
Labels
needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@prashanth26 @gardener-robot @amshuman-kr @AxiomSamarth @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3